Add state sync after call to `_integrate_forces` in `_body_state_changed` #79977

Owl-A · 2023-07-28T04:51:24Z

in issue #79835 , the root cause is that _physics_process is called after _integrate_forces (called from RigidBody3D::_body_state_changed) while _integrate_forces modifies GodotBody3D's linear_velocity, _physics_process reads from now obsolete RigidBody3D::linear_velocity and when we update it using RigidBody3D::set_linear_velocity which also updates GodotBody3D::linear_velocity.

This can be prevented by updating RigidBody3D's mutable state as soon as we return from _integrate_forces, which is what I have added.

Bugsquad edit, fixes: [GodotPhysics] Unexpected behavior while interacting with linear_velocity and state.linear_velocity. #79835

mihe · 2023-07-28T07:48:01Z

scene/3d/physics_body_3d.cpp

+	set_global_transform(p_state->get_transform());
+
+	linear_velocity = p_state->get_linear_velocity();
+	angular_velocity = p_state->get_angular_velocity();
+
+	inverse_inertia_tensor = p_state->get_inverse_inertia_tensor();
+
+	if (sleeping != p_state->is_sleeping()) {
+		sleeping = p_state->is_sleeping();
+		emit_signal(SceneStringNames::get_singleton()->sleeping_state_changed);
+	}
+


It might be a good idea to move all this to a new private method instead, like a void _sync_body_state(PhysicsDirectBodyState3D *p_state), so that you can't accidentally miss one or the other when adding to it. Same goes for the other classes as well.

mihe · 2023-07-28T07:59:13Z

scene/3d/physics_body_3d.cpp

@@ -2925,6 +2937,9 @@ void PhysicalBone3D::_body_state_changed(PhysicsDirectBodyState3D *p_state) {

 	GDVIRTUAL_CALL(_integrate_forces, p_state);

+	linear_velocity = p_state->get_linear_velocity();


I realize you copied the code from above the _integrate_forces call, but this looks odd to me. I don't see why this shouldn't synchronize as much mutable state as possible both before and after _integrate_forces, meaning velocities, sleep state and transform.

As it is right now, it looks like PhysicalBone3D::_integrate_forces doesn't actually get an up-to-date transform like it should be getting.

I was also confused as to why the transform and sleeping were not synced here, should I sync them too?

Also, PhysicalBone3D doesn't seem to have sleeping or inverse_inertia_tensor, we need to only sync transform or am I missing something?

You're right, there is no sleeping (or inverse_inertia_tensor) in PhysicalBone3D, despite there being a can_sleep, so that simplifies things a bit I guess, since it's only the cached state we're interested in syncing.

I can't see a reason why the transform shouldn't be synced, and the set_ignore_transform_notification "scope" should probably span across the _integrate_forces call as well, similar to how it's done in RigidBody3D.

Cool, I'll do that

mihe · 2023-07-31T12:35:23Z

I can't seem to add a comment to these lines, since they're not in proximity of the actual changes, but these lines in PhysicalBone3D::_body_state_changed are now redundant after your latest changes and should be removed:

https://github.com/godotengine/godot/blob/1dbfd2ac5c154c300d44adc9959a895ec28aa75e/scene/3d/physics_body_3d.cpp#L2946-L2948

mihe · 2023-08-01T13:19:51Z

scene/3d/physics_body_3d.cpp

-
-	set_ignore_transform_notification(true);
-	set_global_transform(global_transform);
-	set_ignore_transform_notification(false);
 	_on_transform_changed();


If I can nitpick one last thing it would be to move this _on_transform_changed() in PhysicalBone3D::_body_state_changed up to right below set_ignore_transform_notification(false), similar to how it was before, since they go hand-in-hand.

mihe

I just gave the ragdoll from GDQuest's godot-4-new-features repo a try, and I can confirm that the transform for PhysicalBone3D was indeed stale in _integrate_forces before these changes, and is now as expected.

So this all looks good to me.

(I assume someone in a more official capacity will need to also look over these changes though.)

rburing

This makes sense to me, and fixes the bug.

The behavior change should be mentioned in the release notes. To get the old behavior back (which probably nobody really wants) you would have to do your own caching.

Note that the force integration callback is currently only called using

godot/scene/3d/physics_body_3d.cpp

Line 503 in dcd187d

GDVIRTUAL_CALL(_integrate_forces, p_state);

in the scene/ code here, and it doesn't go through the physics server, i.e. body_set_force_integration_callback is currently never called from scene/ code, and even if it were, the callback is never used (at least in Godot Physics). I documented this when I noticed it, but I think this should be fixed as well (in another PR), so that bodies created in the physics server can have a force integration callback too.

YuriSizov · 2023-08-01T14:55:52Z

The behavior change should be mentioned in the release notes. To get the old behavior back (which probably nobody really wants) you would have to do your own caching.

@rburing Could you quickly summarize what kind of behavior is affected?

mihe · 2023-08-01T15:03:05Z

and even if it were, the callback is never used (at least in Godot Physics)

@rburing Not to derail this PR too much, but this is not correct. I can't speak for 2D, but the code looks very similar to the 3D one, where the callback that's set using body_set_force_integration_callback is invoked as part of GodotBody3D::call_queries.

The Godot Physics implementation does however only invoke the callback if you provide a piece of userdata along with the callback, as seen in the link there, which is likely a bug and might be why you thought it wasn't used.

The documentation for body_set_force_integration_callback also incorrectly states that this callback is conditionally invoked based on body_set_omit_force_integration, which is also not the case. I'm not sure what the original intent was, but I would argue they shouldn't affect eachother, since RigidBody3D._integrate_forces is called even when not using body_set_omit_force_integration (a.k.a. RigidBody3D.custom_integrator).

However, your overall point about these custom integration callbacks being left out of the synchronization that this PR improves upon is a good one. I'm not sure how you would fix this to be honest, since the custom integration callback is invoked before the body_state_callback, which is what calls _body_state_changed, meaning you'll always end up with stale cached state in any such custom integration callback.

YuriSizov · 2023-08-01T18:08:52Z

Thanks!

rburing · 2023-08-02T10:45:21Z

@YuriSizov

Could you quickly summarize what kind of behavior is affected?

A physics body node has state (of the corresponding body in the physics server) and properties (on the node). The order of processing in one physics tick used to be: (1) the equivalent of _sync_body_state, (2) _integrate_forces, (3) _physics_process. So when _integrate_forces changed the body's state on tick number n, that state would be copied to the body node's properties by (the equivalent of) _sync_body_state on tick n+1, too late to be seen in the _physics_process call on tick n. Moreover, incremental updates to properties like linear_velocity += ... (i.e. depending on the "current" value) in _physics_process would be using a stale value, causing bugs like the one this PR fixes. This PR adds another _sync_body_state after (2) _integrate_forces, so that the body node's properties are up-to-date in _physics_process on tick n, even if the state was modified by _integrate_forces.

So it affects the behavior of physics body scripts overriding both _physics_process and _integrate_forces.

By the way, writing the above explanation makes me think _sync_body_state should just only be after _integrate_forces and not before it, since inside that method you should just use the state parameter and not the node's properties.

@mihe Sorry, I got mixed up when writing that doc, I'll fix it (plus the other documentation errors you mentioned). The callback that can be set using body_set_force_integration_callback does get called as you show, but body_set_force_integration_callback itself never gets called by scene/ code. I would expect the rigid body scene/ code to call it with a callback that calls _integrate_forces. But maybe the current timing is better? I don't know.

meaning you'll always end up with stale cached state in any such custom integration callback

The custom integration callback takes the physics server body's state as input, so I don't think there's an issue with stale state there.

mihe · 2023-08-02T11:58:14Z

The order of processing in one physics tick used to be: (1) _physics_process, (2) the equivalent of _sync_body_state, (3) _integrate_forces

Maybe I'm misunderstanding your frame numbering, but _integrate_forces (and thus also _sync_body_state) comes before _physics_process, as seen here, where flush_queries is what ends up calling _body_state_changed and thus _integrate_forces.

So a frame used to be:

Call any custom force integration callback
_body_state_changed
- Update cached state in RigidBody3D
- _integrate_forces
_physics_process
Step the simulation
Maybe go back to 1 a couple of times
_process and whatever else

Meaning if _integrate_forces made any changes through PhysicsDirectBodyState3D (thereby bypassing RigidBody3D) then the cached state in RigidBody3D would be stale in _physics_process.

Now instead a frame is:

Call any custom force integration callback
_body_state_changed
- Update cached state in RigidBody3D
- _integrate_forces
- Update cached state in RigidBody3D
_physics_process
Step the simulation
Maybe go back to 1 a couple of times
_process and whatever else

I would expect the rigid body scene/ code to call it with a callback that calls _integrate_forces. But maybe the current timing is better? I don't know.

I guess if one considers body_set_force_integration_callback to be a scene-independent way of achieving the equivalent of _integrate_forces then it should probably remain the responsibility of the physics server to call it.

The custom integration callback takes the physics server body's state as input, so I don't think there's an issue with stale state there.

I guess the scenario I had in mind was if somebody used this together with a RigidBody3D, and passed the body as the callback userdata, in which case that body would not have had the chance to update its cached state yet. I've seen someone mention this use-case before, where they had some manager-type object that dealt with some collection of bodies, and for whatever reason wanted to do the force-integration from the manager, and thus reached for body_set_force_integration_callback. But I suppose if you start mixing scene stuff with server stuff then you'll just have to accept whatever sharp edges there might be.

Owl-A requested review from a team as code owners July 28, 2023 04:51

Owl-A force-pushed the bugfix branch from d28b8bb to c8655a9 Compare July 28, 2023 04:58

AThousandShips added bug topic:physics labels Jul 28, 2023

AThousandShips requested a review from a team July 28, 2023 07:26

mihe reviewed Jul 28, 2023

View reviewed changes

AThousandShips changed the title ~~added state sync after call to _integrate_forces in _body_state_changed to remedy issue #79835~~ Added state sync after call to _integrate_forces in _body_state_changed Jul 28, 2023

AThousandShips added this to the 4.2 milestone Jul 28, 2023

Owl-A force-pushed the bugfix branch 2 times, most recently from 9bf784f to 1dbfd2a Compare July 31, 2023 11:23

Owl-A force-pushed the bugfix branch from 1dbfd2a to a93c5c3 Compare July 31, 2023 13:06

mihe reviewed Aug 1, 2023

View reviewed changes

added state sync after call to _integrate_forces

c118256

Owl-A force-pushed the bugfix branch from a93c5c3 to c118256 Compare August 1, 2023 13:34

mihe approved these changes Aug 1, 2023

View reviewed changes

rburing approved these changes Aug 1, 2023

View reviewed changes

YuriSizov changed the title ~~Added state sync after call to _integrate_forces in _body_state_changed~~ Add state sync after call to _integrate_forces in _body_state_changed Aug 1, 2023

YuriSizov merged commit e810671 into godotengine:master Aug 1, 2023

rburing mentioned this pull request Aug 2, 2023

Fix physics server force integration docs #80166

Merged

jrouwe mentioned this pull request Sep 26, 2023

Fix performance regression in RigidBody2D/3D and PhysicalBone3D #82393

Merged

awardell mentioned this pull request Oct 9, 2023

Getting global property while Setting local property causes _integrate_physics to malfunction #83029

Open

akien-mga mentioned this pull request Oct 31, 2023

look_at() doesn't work with RigidBody3D #83412

Closed

Owl-A deleted the bugfix branch October 31, 2023 09:43

This was referenced Nov 12, 2023

Fix transform changes in _integrate_forces being overwritten #84799

Merged

Fix transform notification not getting sent out for RigidBody2D (reverted) #84856

Merged

RigidBody2D overwrites most transform changes done in _integrate_forces #84919

Closed

mihe mentioned this pull request Dec 4, 2023

_integrate_forces / KinematicCollision3D bug #85561

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add state sync after call to `_integrate_forces` in `_body_state_changed` #79977

Add state sync after call to `_integrate_forces` in `_body_state_changed` #79977

Owl-A commented Jul 28, 2023 •

edited by AThousandShips

Loading

mihe Jul 28, 2023

mihe Jul 28, 2023 •

edited

Loading

Owl-A Jul 31, 2023

Owl-A Jul 31, 2023 •

edited

Loading

mihe Jul 31, 2023

Owl-A Jul 31, 2023

mihe commented Jul 31, 2023

mihe Aug 1, 2023 •

edited

Loading

mihe left a comment •

edited

Loading

rburing left a comment

YuriSizov commented Aug 1, 2023

mihe commented Aug 1, 2023

YuriSizov commented Aug 1, 2023

rburing commented Aug 2, 2023 •

edited

Loading

mihe commented Aug 2, 2023

		@@ -2925,6 +2937,9 @@ void PhysicalBone3D::_body_state_changed(PhysicsDirectBodyState3D *p_state) {

		GDVIRTUAL_CALL(_integrate_forces, p_state);

		linear_velocity = p_state->get_linear_velocity();

Add state sync after call to _integrate_forces in _body_state_changed #79977

Add state sync after call to _integrate_forces in _body_state_changed #79977

Conversation

Owl-A commented Jul 28, 2023 • edited by AThousandShips Loading

mihe Jul 28, 2023

Choose a reason for hiding this comment

mihe Jul 28, 2023 • edited Loading

Choose a reason for hiding this comment

Owl-A Jul 31, 2023

Choose a reason for hiding this comment

Owl-A Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

mihe Jul 31, 2023

Choose a reason for hiding this comment

Owl-A Jul 31, 2023

Choose a reason for hiding this comment

mihe commented Jul 31, 2023

mihe Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

mihe left a comment • edited Loading

Choose a reason for hiding this comment

rburing left a comment

Choose a reason for hiding this comment

YuriSizov commented Aug 1, 2023

mihe commented Aug 1, 2023

YuriSizov commented Aug 1, 2023

rburing commented Aug 2, 2023 • edited Loading

mihe commented Aug 2, 2023

Add state sync after call to `_integrate_forces` in `_body_state_changed` #79977

Add state sync after call to `_integrate_forces` in `_body_state_changed` #79977

Owl-A commented Jul 28, 2023 •

edited by AThousandShips

Loading

mihe Jul 28, 2023 •

edited

Loading

Owl-A Jul 31, 2023 •

edited

Loading

mihe Aug 1, 2023 •

edited

Loading

mihe left a comment •

edited

Loading

rburing commented Aug 2, 2023 •

edited

Loading